Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Dec 12, 2024

This PR fixes #1226 by calling frankenphp.Shutdown() on Caddy's Shutdown().

I also went ahead and replaced the CaddyUsagePool by a global boolean. It's probably simpler to just have Shutdown() do nothing if FrankenPHP is not running (unless I'm missing something).

@ghost ghost requested a review from dunglas December 12, 2024 22:19
@AlliBalliBaba AlliBalliBaba changed the title Shuts caddy down gracefully. fix - graceful shutdown Dec 12, 2024
@AlliBalliBaba
Copy link
Contributor

As a sidenote: I'm still not sure why calling frankenphp.Shutdown() inside Stop() will make Caddy's integration tests fail, but will work normally outside of tests. It seems like caddytest will call Stop() too early, preventing requests from being properly finished.


// attempt a graceful shutdown if we are not running tests
if flag.Lookup("test.v") == nil {
frankenphp.Shutdown()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cannot work because in case of reload, Caddy starts the new module before stopping the old one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I think I get it now. There's an experimental caddy.Exiting() api that we could maybe use instead.

Is there also a way to reload Caddy outside of tests? Running curl "http://localhost:2019/load" via the admin API also doesn't work on main.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nvm I realized you have to actually patch the whole config for a reload

curl "http://localhost:2019/config/apps/frankenphp" -X PATCH --header "Content-Type: application/json" -d '{"workers"
:[{"file_name":"/go/src/app/testdata/index.php","watch":["./**/*.{php,yaml,yml,twig,env}"]}]}' -H "Cache-Control: must-revalidate"

This works now with the Exiting() check if you're fine with using an experimental api. Otherwise I'll close this PR.

Copy link
Contributor

@francislavoie francislavoie Dec 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use --force if you're using caddy reload or equivalent, which is the same as passing the Cache-Control: must-revalidate header in the API to reload even if the config text has not changed.

Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@dunglas
Copy link
Member

dunglas commented Dec 17, 2024

Could you please rebase?

@dunglas dunglas merged commit fbbc129 into main Dec 17, 2024
53 checks passed
@dunglas dunglas deleted the fix/graceful-shutdown branch December 17, 2024 17:10
@dunglas
Copy link
Member

dunglas commented Dec 17, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Worker script itself is not shutdown gracefully, but hangs

4 participants